Update schema to support oneOf, and normalize objects by apelisse · Pull Request #72 · kubernetes-sigs/structured-merge-diff

apelisse · 2019-03-20T17:41:20Z

This does mostly two things:

Defines the schema for OneOf: multiple oneof can be at the same level (in the same structure),
Normalizes an object based on oneofs definition: uses the discriminator if present and has changed, uses the difference of fields if they changed, updates the discriminator if present.

And adds tests. This is still not plugged anyway though, so it's not going to do anything.

k8s-ci-robot · 2019-03-20T17:41:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [apelisse]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apelisse · 2019-03-20T17:41:36Z

/assign @lavalamp @kwiesmueller @jennybuckley

typed/deduced.go

schema/elements.go

typed/typed.go

jennybuckley · 2019-03-20T21:37:39Z

typed/union.go

@@ -0,0 +1,130 @@
+/*
+Copyright 2018 The Kubernetes Authors.


jennybuckley · 2019-03-20T21:37:56Z

typed/union_test.go

@@ -0,0 +1,224 @@
+/*
+Copyright 2018 The Kubernetes Authors.


jennybuckley · 2019-03-20T22:07:33Z

typed/union.go

+	return set
+}
+
+// Set the discriminator to a specific value


jennybuckley · 2019-03-20T22:09:10Z

typed/union.go

+	if union.Discriminator != nil {
+		df = uncapitalize(*union.Discriminator)
+		odv := getDiscriminatorValue(old, df)
+		ndv := getDiscriminatorValue(new, df)


Do we also need to uncapitalize the odv and ndv? It seems a bit kubernetes specific.

jennybuckley · 2019-03-20T22:14:23Z

typed/union_test.go

+    unions:
+      - discriminator: discriminator
+        fields: [a, b, c]
+      - fields: [one, two, three]`)


Test looks good, maybe we should also try some nested structs

Also we should test that creating a new object should still set the discriminator correctly

jennybuckley · 2019-03-20T22:24:00Z

typed/union.go

+
+	for field := range ofs {
+		if _, ok := nfs[field]; !ok {
+			out.MapValue.Delete(field)


Maybe we could use the same

for _, field := range union.Fields { if field != ndv { out.MapValue.Delete(field) } }

loop from before (or a function that does that) and not worry about ofs or nfs anymore?

jennybuckley · 2019-03-20T22:24:59Z

typed/union.go

+	}
+
+	if df != "" {
+		out.MapValue.Set(df, value.StringValue(dv))


value.StringValue(capitalize(dv))?

typed/union.go

lavalamp · 2019-03-21T18:47:23Z

schema/elements.go

 	// one or more fields in the above list. A given field from the above
 	// list may be referenced in exactly 0 or 1 places in the below list.
-	// Unions []Union `yaml:"unions,omitempty"`
+	Unions []Union `yaml:"unions,omitempty"`


One of the reasons this was commented out is that I wasn't sure if I wanted to do it the way you've added it, or if it would be better to use a struct per union and support embedding a list of structs. The other way would make it impossible to try and put the same field into multiple unions. I'm not sure where, but it'd be nice to see a discussion of why this format is chosen. (Maybe you have it and I haven't seen it yet.)

The way it's going, we're only going to accept one union per structure in the OpenAPI, which would prevent any sort of overlap between fields

lavalamp · 2019-03-21T18:49:25Z

schema/elements.go

 	// one or more fields in the above list. A given field from the above
 	// list may be referenced in exactly 0 or 1 places in the below list.
-	// Unions []Union `yaml:"unions,omitempty"`
+	Unions []Union `yaml:"unions,omitempty"`


Also consider a different name, since you have to explain "Union". Maybe Exclusivities or OneOfs?

lavalamp

Super exciting change, I especially appreciate the thorough tests!

lavalamp · 2019-03-21T18:49:46Z

schema/elements.go

 }

+// Union, or oneof, means that only one of multiple fields of a structure can be
+// set at all times. For backward compatibility reasons, and to help "dumb


Suggested change

// set at all times. For backward compatibility reasons, and to help "dumb

// set at a time. For backward compatibility reasons, and to help "dumb

lavalamp · 2019-03-21T18:50:04Z

schema/elements.go

+// set at all times. For backward compatibility reasons, and to help "dumb
+// clients" that are not aware of union (or can't be aware of this because they
+// don't know what fields are part of the union), the code tolerates multiple
+// fields to be set but will try to detect which fields must be cleaned (there


Suggested change

// fields to be set but will try to detect which fields must be cleaned (there

// fields to be set but will try to detect which fields must be cleared (there

lavalamp · 2019-03-21T18:50:36Z

schema/elements.go

+// don't know what fields are part of the union), the code tolerates multiple
+// fields to be set but will try to detect which fields must be cleaned (there
+// should never be more than two though):
+// - If the Discriminator field exist, the value of the field will be used to


Suggested change

// - If the Discriminator field exist, the value of the field will be used to

// - If the Discriminator field exists, the value of the field will be used to

lavalamp · 2019-03-21T18:52:11Z

schema/elements.go


+// Union, or oneof, means that only one of multiple fields of a structure can be
+// set at all times. For backward compatibility reasons, and to help "dumb
+// clients" that are not aware of union (or can't be aware of this because they


Suggested change

// clients" that are not aware of union (or can't be aware of this because they

// clients" which are not aware of the union (or can't be aware of it, because they

lavalamp · 2019-03-21T18:53:07Z

schema/elements.go

+// fields to be set but will try to detect which fields must be cleaned (there
+// should never be more than two though):
+// - If the Discriminator field exist, the value of the field will be used to
+// decide which field to clean and which field to keep


Suggested change

// decide which field to clean and which field to keep

// decide which field to clear and which field to keep

lavalamp · 2019-03-21T19:03:12Z

typed/union_test.go

+		{
+			name: "client sends multiple with discriminator",
+			old:  `{}`,
+			new:  `{"a": 1, "b": 1, "discriminator": "a"}`,


I also think this should cause an error.

Actually, the use-case in mind here is the following:

I'm using kustomize

my base deployment uses an emptyDir

my overlay uses a persistent disk

kustomize typically merges this by having both (you have to explicitely set emptyDir to null)

In that case, you could use the discriminator to clarify (which is much better than setting discriminator to null)

It sounds like kustomize should use this library to do the merges. It'd be pretty nifty if customize produced output which had each overlay as a separate field manager.

So this would mean someone could send

deploymentStrategyType: Recreate rollingUpdate: maxUnavailable: 1

and it would be accepted and treated as this now?

deploymentStrategyType: Recreate rollingUpdate: {}

lavalamp · 2019-03-21T19:03:53Z

typed/union_test.go

+			name: "change discriminator, nothing else",
+			old:  `{"discriminator": "a"}`,
+			new:  `{"discriminator": "random"}`,
+			out:  `{"discriminator": "random"}`,


random discriminator values should cause an error, don't you think?

I think this is similar to the one below, which is to make sure changing a DeploymentStrategyType from RollingUpdate to Recreate clears the RollingUpdate field.
Recreate isn't a name of a field in the Union but it is an allowed value of DeploymentStrategyType. If it's not allowed as a DeploymentStrategyType, I think validation outside smd could handle that kind of thing.

lavalamp · 2019-03-21T19:04:29Z

typed/union_test.go

+			name: "remove discriminator, nothing else",
+			old:  `{"discriminator": "a", "a": 1}`,
+			new:  `{"a": 1}`,
+			out:  `{"a": 1}`,


I guess I would have expected a to be cleared also--not 100% sure.

lavalamp · 2019-03-21T19:05:22Z

typed/union_test.go

+		{
+			name: "new object has three of same union set",
+			old:  `{"a": 1}`,
+			new:  `{"a": 2, "b": 1, "c": 3}`,


This should cause an error, I think.

I think this would cause an error outside smd as well

lavalamp · 2019-03-21T19:06:20Z

typed/union_test.go

+		{
+			name: "old object has two of same union set",
+			old:  `{"a": 1, "b": 2}`,
+			new:  `{"a": 2, "b": 1}`,


I'd like a test case exactly like this except only one of the values changes.

schema/elements.go

jennybuckley · 2019-04-02T23:51:40Z

schema/elements.go

+	// parent structure. Discriminator (if oneOf has one), is NOT included
+	// in this list. The value for field is how we map the name of the field
+	// to actual value for discriminator.
+	Fields map[string]string `yaml:"fields,omitempty"`


can a value be mapped to from multiple keys?

Also maybe this should be a list, since we use lists in all the other schema pieces

Also maybe this should be a list, since we use lists in all the other schema pieces

Good point

can a value be mapped to from multiple keys?

No, it shouldn't. The part that converts the openapi into this schema is going to verify that this is not possible. If it still happens that we have two mappings going to the same key, it will probably use the last one in the list.

jennybuckley · 2019-04-08T17:51:34Z

schema/elements.go

+	FieldName string `yaml:"fieldName"`
+	// DiscriminatedBy is the value of the discriminator to select that
+	// field.
+	DiscriminatedBy string `yaml:"DiscriminatedBy"`


What would this field be set to in the case of Unions that don't have a discriminator?

jennybuckley · 2019-04-08T17:52:09Z

schema/elements.go

+	Discriminator *string `yaml:"discriminator,omitempty"`
+
+	// This is the list of fields that belong to this union. This fields are
+	// required to not be part of another union, or be the discriminator for


Only one union now

apelisse · 2019-04-08T19:48:59Z

Fixed

typed/union.go

typed/union_test.go

schema/elements.go

Normalization can return errors if something very wrong is happening, like setting two new fields, or if setting a new field while also changing the discriminator.

jennybuckley · 2019-04-16T23:03:53Z

/lgtm

apelisse · 2019-04-17T03:16:09Z

I forgot to cc @sttts, I'll happily update code based on your comments Stefan!

k8s-ci-robot requested review from jennybuckley and lavalamp March 20, 2019 17:41

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 20, 2019

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2019

k8s-ci-robot assigned jennybuckley, kwiesmueller and lavalamp Mar 20, 2019

jennybuckley reviewed Mar 20, 2019

View reviewed changes

apelisse commented Mar 20, 2019

View reviewed changes

typed/union.go Outdated Show resolved Hide resolved

lavalamp reviewed Mar 21, 2019

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 22, 2019

apelisse mentioned this pull request Mar 28, 2019

Initial KEP for API union/oneOf kubernetes/enhancements#926

Merged

apelisse force-pushed the oneof branch from 2424bea to 5556117 Compare April 2, 2019 15:40

jennybuckley mentioned this pull request Apr 2, 2019

Umbrella Issue for Server Side Apply kubernetes/kubernetes#73723

Closed

apelisse mentioned this pull request Apr 2, 2019

Add parsing of openapi oneof/union semantic into smd kubernetes/kube-openapi#152

Merged

apelisse force-pushed the oneof branch from 5556117 to 26adf6a Compare April 2, 2019 23:03

jennybuckley reviewed Apr 2, 2019

View reviewed changes

apelisse force-pushed the oneof branch 2 times, most recently from 4f934de to ceb39c1 Compare April 8, 2019 17:33

jennybuckley reviewed Apr 8, 2019

View reviewed changes

apelisse force-pushed the oneof branch from 2da4f5b to 9dc837e Compare April 8, 2019 19:48

jennybuckley reviewed Apr 9, 2019

View reviewed changes

typed/union.go Show resolved Hide resolved

typed/union_test.go Show resolved Hide resolved

jennybuckley reviewed Apr 9, 2019

View reviewed changes

schema/elements.go Show resolved Hide resolved

Antoine Pelisse added 3 commits April 10, 2019 12:18

Define new schema for Unions

20836a5

Add method to delete items from maps

1078cd7

Implement unions normalization

ff00828

Normalization can return errors if something very wrong is happening, like setting two new fields, or if setting a new field while also changing the discriminator.

apelisse force-pushed the oneof branch from 9dc837e to ff00828 Compare April 10, 2019 19:19

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2019

k8s-ci-robot merged commit b2ed7e1 into kubernetes-sigs:master Apr 16, 2019

jpbetz mentioned this pull request Aug 28, 2025

API Unions kubernetes/enhancements#1027

Open

	// set at all times. For backward compatibility reasons, and to help "dumb
	// set at a time. For backward compatibility reasons, and to help "dumb

	// fields to be set but will try to detect which fields must be cleaned (there
	// fields to be set but will try to detect which fields must be cleared (there

	// - If the Discriminator field exist, the value of the field will be used to
	// - If the Discriminator field exists, the value of the field will be used to

	// clients" that are not aware of union (or can't be aware of this because they
	// clients" which are not aware of the union (or can't be aware of it, because they

	// decide which field to clean and which field to keep
	// decide which field to clear and which field to keep

Conversation

apelisse commented Mar 20, 2019

Uh oh!

k8s-ci-robot commented Mar 20, 2019

Uh oh!

apelisse commented Mar 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Apr 8, 2019

apelisse Apr 3, 2019 •

edited

Loading